Skip to content

Re-enable ThinLTO for rustc on x86_64-apple-darwin #105845

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 19, 2022
Merged

Re-enable ThinLTO for rustc on x86_64-apple-darwin #105845

merged 1 commit into from
Dec 19, 2022

Conversation

lqd
Copy link
Member

@lqd lqd commented Dec 17, 2022 •
edited
Loading

ThinLTO was disabled on x64 mac in #105646 because of the #105637 regression.

It was later discovered that the issue was present on other targets as well, as the mac revert was already landing. The linux/win reverts, however, did not land before the root cause was identified.

#105800 fixed the underlying issue in -Zdylib-lto handling, and the x64 msvc and linux targets are now fixed, ICEs are using the correct rustc_driver panic hook.

This PR re-enables ThinLTO on mac for improved perf now that the issue should be fixed everywhere.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 17, 2022
@lqd
Copy link
Member Author

lqd commented Dec 17, 2022

@bors try

@bors
Copy link
Collaborator

bors commented Dec 17, 2022

⌛ Trying commit a270a348bc99a57e25b39e58db466f928f03439b with merge 16aba2156f3ed344308e345c000744d0476fd9ec...

@Mark-Simulacrum
Copy link
Member

r=me on the re-enable presuming try builds look good

@bors
Copy link
Collaborator

bors commented Dec 17, 2022

☀️ Try build successful - checks-actions
Build commit: 16aba2156f3ed344308e345c000744d0476fd9ec (16aba2156f3ed344308e345c000744d0476fd9ec)

@lqd lqd force-pushed the revert-thinlto-revert branch from a270a34 to ae68e17 Compare December 18, 2022 10:16
@lqd lqd marked this pull request as ready for review December 18, 2022 10:29
@lqd
Copy link
Member Author

lqd commented Dec 18, 2022

This is looking good, the 16aba2156f3ed344308e345c000744d0476fd9ec build correctly displays the ICE from #105777 like nightly does. It does not print correctly with the ThinLTOed nightly-2012-12-12 where the issue was found.

I've dropped the temporary CI builder hack.

For bisection and perf we can avoid rolling it up, but it shouldn't really matter.

@bors r=Mark-Simulacrum rollup=never

@bors
Copy link
Collaborator

bors commented Dec 18, 2022

📌 Commit ae68e17 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2022
@bors
Copy link
Collaborator

bors commented Dec 18, 2022

⌛ Testing commit ae68e17 with merge f6f6328ec504cb9ab8ef1234282ff206d65f3657...

@bors
Copy link
Collaborator

bors commented Dec 18, 2022

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 18, 2022
@lqd
Copy link
Member Author

lqd commented Dec 18, 2022

@bors retry - x86_64-gnu-llvm-13-stage1 was unresponsive while compiling rustbuild right at the beginning

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2022
@rust-log-analyzer

This comment was marked as outdated.

# to view
@bors
Copy link
Collaborator

bors commented Dec 19, 2022

⌛ Testing commit ae68e17 with merge 1072337...

@bors
Copy link
Collaborator

bors commented Dec 19, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 1072337 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2022
@bors bors merged commit 1072337 into rust-lang:master Dec 19, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 19, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1072337): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 5.6% [5.6%, 5.6%] 1
Improvements ✅
(primary)		 -2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)		 -2.1% [-4.2%, -0.6%] 21
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@lqd lqd deleted the revert-thinlto-revert branch December 19, 2022 11:21
@lqd lqd mentioned this pull request Jun 22, 2023
Open
30 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@lqd @bors @Mark-Simulacrum @rust-log-analyzer @rust-timer @rustbot